Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstraction for LO frequency to MWChannel that matches the IQChannel #75

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

deanpoulos
Copy link
Collaborator

For OPX+/OPX1000 dynamic code, we would like the IQChannel and MWChannel to share a common interface for accessing the upconverter frequency field.

Although this approach matches the convention used in the IQChannel, I'm indifferent to how the property looks at the end of the day, as long as the upconverter frequency can be accessed the same way between both channel types.

@nulinspiratie @KevinAVR

@nulinspiratie
Copy link
Contributor

nulinspiratie commented Sep 10, 2024

Thanks for the PR @deanpoulos, I think we'll need something like this, but let's think a bit about which properties need to be shared. We may also want to share other properties such as RF_frequency, as well as the inferred frequencies

@deanpoulos
Copy link
Collaborator Author

Thanks for the PR @deanpoulos, I think we'll need something like this, but let's think a bit about which properties need to be shared. We may also want to share other properties such as RF_frequency, as well as the inferred frequencies

Sure thing @nulinspiratie. This is a minimal change to unblock ~5 calibration nodes at the moment. RF_frequency would have the same effect. Let's talk after IEEE. Do you propose some alternative, or will you take it?

@deanpoulos
Copy link
Collaborator Author

@TheoLaudatQM

@nulinspiratie
Copy link
Contributor

@deanpoulos the changes you propose should be added to QuAM, but I would like to merge it once it also has RF_frequency, and the inferred functions implemented. I can implement these changes next week

@deanpoulos
Copy link
Collaborator Author

Hey @nulinspiratie so you will take it from here?

@nulinspiratie
Copy link
Contributor

Thanks for the reminder! Will aim to have it ready for review first half of this week

@nulinspiratie
Copy link
Contributor

@deanpoulos added RF_frequency, which by default is inferred from intermediate_frequency and LO_frequency.
Can't add you as a reviewer since you started the PR, but can you go over it and see if it works?

Only had time to add a basic test, will add more at some point in the future, but not in this PR

@deanpoulos
Copy link
Collaborator Author

Looking into it today @nulinspiratie

@deanpoulos
Copy link
Collaborator Author

@deanpoulos added RF_frequency, which by default is inferred from intermediate_frequency and LO_frequency. Can't add you as a reviewer since you started the PR, but can you go over it and see if it works?

Only had time to add a basic test, will add more at some point in the future, but not in this PR

Hey @nulinspiratie, I went initialized a QuAM with your fix included for flux-tunable qubits, and ran through the regular post-processing on one of the calibration nodes using dummy data.

I can confirm that it worked without error, and the post-processing (frequency updates) worked as expected. I think this is good enough to merge for now, and we will obviously test it further in the field with real data.

@nulinspiratie nulinspiratie self-requested a review October 11, 2024 17:22
@nulinspiratie nulinspiratie merged commit d6a03ad into main Oct 11, 2024
2 checks passed
@nulinspiratie nulinspiratie deleted the feature/upconverter_freq_mw_channel_abstraction branch October 11, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants